FIX: Preserve Arrow reader fetch exceptions (#712) by subrata-ms · Pull Request #718 · microsoft/mssql-python

Subrata (subrata-ms) · 2026-08-13T05:47:30Z

Work Item / Issue Reference

AB#47281

GitHub Issue: #712

Summary

This pull request improves the robustness and test coverage of the cursor cleanup logic in the mssql_python package, particularly around error handling during fetch operations and resource cleanup. It also introduces a build check to ensure that the driver source code compiles cleanly with warnings treated as errors in Python 3.14 and above.

Error handling and cleanup improvements:

Refactored the cleanup guard in batch_generator() in cursor.py to only skip cleanup if the cursor is None, closed, or has no hstmt, improving clarity and correctness.

Test coverage enhancements:

Added tests in test_004_cursor_arrow.py to verify that fetch errors are properly propagated and not masked by defensive cleanup logic, both when cleanup is skipped and after normal cleanup. These tests use fake cursor objects to simulate various error and cleanup scenarios.

Build and compatibility checks:

Introduced a new test in test_004_cursor.py to ensure that cursor.py compiles successfully with SyntaxWarning promoted to an error, as required by Python 3.14+ (PEP 765). This helps future-proof the codebase against upcoming Python changes.
Added necessary imports (subprocess, sys, Path) in test_004_cursor.py to support the new compilation test.

Copilot

Pull request overview

This PR fixes Cursor.arrow_reader() cleanup semantics in mssql_python/cursor.py so fetch exceptions from the Arrow batch generator are not accidentally discarded, addressing Python 3.14+ warnings-as-errors behavior (PEP 765) and improving robustness around teardown paths.

Changes:

Refactors the arrow_reader() batch generator finally cleanup guard to avoid return in finally and preserve in-flight fetch exceptions.
Adds Arrow-reader tests that assert fetch errors propagate both when cleanup is skipped and when cleanup runs normally.
Adds a Python 3.14+ test that compiles mssql_python/cursor.py under warnings-as-errors to catch future “return in finally” regressions.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
mssql_python/cursor.py	Removes the `return`-in-`finally` pattern in Arrow reader cleanup by converting it to a conditional cleanup block, preserving exceptions.
tests/test_004_cursor_arrow.py	Adds regression tests ensuring Arrow reader fetch errors are not masked by defensive cleanup paths.
tests/test_004_cursor.py	Adds a Python 3.14+ compilation test to ensure `cursor.py` compiles cleanly with warnings promoted to errors.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

github-actions · 2026-08-17T06:51:29Z

📊 Code Coverage Report

🔥 Diff Coverage 100%	🎯 Overall Coverage 82%	📈 Total Lines Covered: `7750` out of `9429` 📁 Project: `mssql-python`

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

mssql_python/cursor.py (100%)

Summary

Total: 14 lines
Missing: 0 lines
Coverage: 100%

📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)

mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 75.5%
mssql_python.__init__.py: 77.6%
mssql_python.row.py: 77.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.4%
mssql_python.pybind.connection.connection.cpp: 84.3%
mssql_python.logging.py: 85.5%

🔗 Quick Links

⚙️ Build Summary	📋 Coverage Details
View Azure DevOps Build	Browse Full Coverage Report

Gaurav Sharma (bewithgaurav)

requesting changes on fake cursor tests, a suggestion, and a nit comment
the core fix lgtm

Gaurav Sharma (bewithgaurav) · 2026-08-17T07:17:03Z

-                    cur.messages.extend(ddbc_bindings.DDBCSQLGetAllDiagRecords(cur.hstmt))
-                except Exception as e:  # pylint: disable=broad-exception-caught
-                    logger.debug("arrow_reader cleanup: post-close diag drain failed: %s", e)
+                if cur is not None and not cur.closed and cur.hstmt is not None:


nit: cur is never None here, it gets read on the line right above and this block runs once per reader
so if not cur.closed and cur.hstmt is not None: would do the same, only mentioning it since the line is already changing

Gaurav Sharma (bewithgaurav) · 2026-08-17T07:19:43Z

+@pytest.mark.skipif(sys.version_info < (3, 14), reason="PEP 765 warnings begin in Python 3.14")
+def test_cursor_compiles_with_warnings_as_errors():
+    """The driver source must compile when SyntaxWarning is promoted to an error."""
+    cursor_source = Path(__file__).parents[1] / "mssql_python" / "cursor.py"


this checks cursor.py only, but what broke was importing the package
the same return in a finally in any other file breaks it the same way, and this test would stay green

looping over the package covers all of it:

package_dir = Path(__file__).parents[1] / "mssql_python" for source in sorted(package_dir.glob("*.py")): with warnings.catch_warnings(): warnings.simplefilter("error") compile(source.read_text(encoding="utf-8"), str(source), "exec")

ran it on 3.14 and 3.13, green on both. so the skipif can go and it covers every leg instead of just the 3.14 ones. needs import warnings, and subprocess becomes unused

Gaurav Sharma (bewithgaurav) · 2026-08-17T07:43:54Z

+@pytest.mark.parametrize(
+    ("closed", "has_hstmt"),
+    [(True, True), (False, False)],
+    ids=["closed-cursor", "missing-hstmt"],
+)
+def test_arrow_reader_propagates_fetch_error_when_cleanup_is_skipped(closed, has_hstmt):
+    """A defensive cleanup guard must not turn a fetch error into end-of-stream."""
+
+    class FakeCursor:
+        def __init__(self):
+            self.closed = False
+            self.hstmt = object()
+            self.calls = 0
+
+        def _check_closed(self):
+            pass
+
+        def _ensure_pyarrow(self):
+            return pa
+
+        def arrow_batch(self, _batch_size):
+            self.calls += 1
+            if self.calls == 1:
+                return pa.record_batch([pa.array([], type=pa.int64())], names=["value"])
+
+            self.closed = closed
+            self.hstmt = object() if has_hstmt else None
+            raise RuntimeError("fetch failed")
+
+    fake_cursor = FakeCursor()
+    reader = mssql_python.Cursor.arrow_reader(fake_cursor, batch_size=1)
+    try:
+        with pytest.raises(RuntimeError, match="fetch failed"):
+            reader.read_next_batch()
+    finally:
+        reader.close()
+
+
+def test_arrow_reader_propagates_fetch_error_after_cleanup(monkeypatch):
+    """Fetch errors must survive the normal cleanup path, which must still run."""
+    from mssql_python import cursor as cursor_mod
+
+    class FakeHstmt:
+        def __init__(self):
+            self.close_calls = 0
+
+        def _cancel(self):
+            pass
+
+        def _close_cursor(self):
+            self.close_calls += 1
+
+    class FakeCursor:
+        def __init__(self):
+            self.closed = False
+            self.hstmt = FakeHstmt()
+            self.messages = []
+            self.rowcount = 1
+            self.calls = 0
+            self.rownumber_cleared = False
+
+        def _check_closed(self):
+            pass
+
+        def _ensure_pyarrow(self):
+            return pa
+
+        def _clear_rownumber(self):
+            self.rownumber_cleared = True
+
+        def arrow_batch(self, _batch_size):
+            self.calls += 1
+            if self.calls == 1:
+                return pa.record_batch([pa.array([], type=pa.int64())], names=["value"])
+            raise RuntimeError("fetch failed")
+
+    monkeypatch.setattr(cursor_mod.ddbc_bindings, "DDBCSQLGetAllDiagRecords", lambda _h: [])
+    fake_cursor = FakeCursor()
+    reader = mssql_python.Cursor.arrow_reader(fake_cursor, batch_size=1)
+    try:
+        with pytest.raises(RuntimeError, match="fetch failed"):
+            reader.read_next_batch()
+        assert fake_cursor.hstmt.close_calls == 1
+        assert fake_cursor.rownumber_cleared is True
+        assert fake_cursor.rowcount == -1
+    finally:
+        reader.close()


requesting changes on this one - since these two go green even when the driver is broken.
they never open a connection, so a real regression under the fake object still shows green here

the same bug is testable through the driver: close the cursor part way through a read and assert it raises. fails on the old guard, passes here, adding a suggestion:

Suggested change

@pytest.mark.parametrize(

("closed", "has_hstmt"),

[(True, True), (False, False)],

ids=["closed-cursor", "missing-hstmt"],

)

def test_arrow_reader_propagates_fetch_error_when_cleanup_is_skipped(closed, has_hstmt):

"""A defensive cleanup guard must not turn a fetch error into end-of-stream."""

class FakeCursor:

def __init__(self):

self.closed = False

self.hstmt = object()

self.calls = 0

def _check_closed(self):

pass

def _ensure_pyarrow(self):

return pa

def arrow_batch(self, _batch_size):

self.calls += 1

if self.calls == 1:

return pa.record_batch([pa.array([], type=pa.int64())], names=["value"])

self.closed = closed

self.hstmt = object() if has_hstmt else None

raise RuntimeError("fetch failed")

fake_cursor = FakeCursor()

reader = mssql_python.Cursor.arrow_reader(fake_cursor, batch_size=1)

try:

with pytest.raises(RuntimeError, match="fetch failed"):

reader.read_next_batch()

finally:

reader.close()

def test_arrow_reader_propagates_fetch_error_after_cleanup(monkeypatch):

"""Fetch errors must survive the normal cleanup path, which must still run."""

from mssql_python import cursor as cursor_mod

class FakeHstmt:

def __init__(self):

self.close_calls = 0

def _cancel(self):

pass

def _close_cursor(self):

self.close_calls += 1

class FakeCursor:

def __init__(self):

self.closed = False

self.hstmt = FakeHstmt()

self.messages = []

self.rowcount = 1

self.calls = 0

self.rownumber_cleared = False

def _check_closed(self):

pass

def _ensure_pyarrow(self):

return pa

def _clear_rownumber(self):

self.rownumber_cleared = True

def arrow_batch(self, _batch_size):

self.calls += 1

if self.calls == 1:

return pa.record_batch([pa.array([], type=pa.int64())], names=["value"])

raise RuntimeError("fetch failed")

monkeypatch.setattr(cursor_mod.ddbc_bindings, "DDBCSQLGetAllDiagRecords", lambda _h: [])

fake_cursor = FakeCursor()

reader = mssql_python.Cursor.arrow_reader(fake_cursor, batch_size=1)

try:

with pytest.raises(RuntimeError, match="fetch failed"):

reader.read_next_batch()

assert fake_cursor.hstmt.close_calls == 1

assert fake_cursor.rownumber_cleared is True

assert fake_cursor.rowcount == -1

finally:

reader.close()

_BIG_QUERY = (

"SELECT TOP (3000) ROW_NUMBER() OVER (ORDER BY (SELECT NULL)) AS n "

"FROM sys.all_objects a CROSS JOIN sys.all_objects b"

)

def test_arrow_reader_raises_when_cursor_closes_mid_stream(db_connection):

"""A cursor closed mid-stream must raise, not report a short result set."""

cur = db_connection.cursor()

cur.execute(_BIG_QUERY)

reader = cur.arrow_reader(batch_size=500)

rows = 0

with pytest.raises(mssql_python.Error):

for batch in reader:

rows += batch.num_rows

if rows >= 1000:

cur.close()

assert 0 < rows < 3000

def test_arrow_reader_raises_when_cursor_scope_already_exited(db_connection):

"""A reader outliving its cursor's `with` block must raise, not yield nothing."""

with db_connection.cursor() as cur:

cur.execute(_BIG_QUERY)

reader = cur.arrow_reader(batch_size=500)

with pytest.raises(mssql_python.Error):

for _ in reader:

pass

FIX: Preserve Arrow reader fetch exceptions (#712)

4a7fbb9

Copilot AI lite review requested due to automatic review settings August 13, 2026 05:47

github-actions Bot added the pr-size: medium Moderate update size label Aug 13, 2026

Copilot started reviewing on behalf of Subrata (subrata-ms) August 13, 2026 05:48 View session

Copilot AI reviewed Aug 13, 2026

View reviewed changes

Comment thread tests/test_004_cursor.py

Subrata (subrata-ms) requested review from Gaurav Sharma (bewithgaurav), gargsaumya, Jahnvi Thakkar (jahnvi480) and Sumit Sarabhai (sumitmsft) August 13, 2026 05:51

Subrata (subrata-ms) and others added 5 commits August 12, 2026 22:51

Merge branch 'main' into subrata-ms/Bug712

f12dacb

STYLE: Format Bug 712 cursor test

45e9a75

Potential fix for pull request finding

25bf778

Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Merge branch 'main' into subrata-ms/Bug712

0d633b2

Merge branch 'main' into subrata-ms/Bug712

0d1de40

Merge branch 'main' into subrata-ms/Bug712

c6a3456

Gaurav Sharma (bewithgaurav) requested changes Aug 17, 2026

View reviewed changes

Navigation Menu

Uh oh!

FIX: Preserve Arrow reader fetch exceptions (#712) - #718

FIX: Preserve Arrow reader fetch exceptions (#712)#718
Subrata (subrata-ms) wants to merge 7 commits into
mainfrom
subrata-ms/Bug712

Subrata (subrata-ms) commented Aug 13, 2026 •

edited by azure-boards Bot

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

github-actions Bot commented Aug 17, 2026 •

edited

Loading

🔥 Diff Coverage

100%

🎯 Overall Coverage

82%

Uh oh!

Gaurav Sharma (bewithgaurav) left a comment •

edited

Loading

Uh oh!

Gaurav Sharma (bewithgaurav) Aug 17, 2026

Uh oh!

Gaurav Sharma (bewithgaurav) Aug 17, 2026

Uh oh!

Gaurav Sharma (bewithgaurav) Aug 17, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

Subrata (subrata-ms) commented Aug 13, 2026 • edited by azure-boards Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Work Item / Issue Reference

Summary

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

github-actions Bot commented Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📊 Code Coverage Report

🔥 Diff Coverage

100%

🎯 Overall Coverage

82%

Diff Coverage

Diff: main...HEAD, staged and unstaged changes

Summary

📋 Files Needing Attention

🔗 Quick Links

Uh oh!

Gaurav Sharma (bewithgaurav) left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gaurav Sharma (bewithgaurav) Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

Gaurav Sharma (bewithgaurav) Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

Gaurav Sharma (bewithgaurav) Aug 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Subrata (subrata-ms) commented Aug 13, 2026 •

edited by azure-boards Bot

Loading

github-actions Bot commented Aug 17, 2026 •

edited

Loading

Gaurav Sharma (bewithgaurav) left a comment •

edited

Loading

Gaurav Sharma (bewithgaurav) Aug 17, 2026 •

edited

Loading